Add pokemon form trigger conditions#1578
Conversation
Naramsim
left a comment
Naramsim
left a comment
There was a problem hiding this comment.
Hi, can you merge the migrations into a single one?
Can you address the failing tests?
| 2231,peat-block,10,0,, | ||
| 2232,metal-alloy,10,0,, No newline at end of file | ||
| 2232,metal-alloy,10,0,, | ||
| 2559,clefablite,44,0,80, |
There was a problem hiding this comment.
Why start at 2559 instead of 2233?
| @@ -0,0 +1,224 @@ | |||
| pokemon_form_id,form_method_id,item_id,ability_id,move_id | |||
There was a problem hiding this comment.
Maybe some more descriptive names like trigger_item_id and so on?
| @@ -0,0 +1,7 @@ | |||
| id,identifier | |||
There was a problem hiding this comment.
We should also add a pokemon_form_method_prose.csv file to match
There was a problem hiding this comment.
Is the pokemon_form_trigger_prose.csv the one you are referring to?
There was a problem hiding this comment.
Yes that’s the one that was added, there was some renaming done
| model = PokemonFormName | ||
| fields = ("name", "pokemon_name", "language") | ||
|
|
||
| class PokemonFormTriggerSerializer(serializers.ModelSerializer): |
There was a problem hiding this comment.
I do like form trigger as the name rather than form method, but whatever you choose we should make it consistent between these classes and the file.
|
The new update addresses the comments to fix the ID and naming issues. Migrations is merged into single file and prose csv file is added |
|
I'm loving the format and after checking a few pokemon things are looking good! This next part is a bit out of my expertise so I'll tag @Naramsim - would it be good to have this as a separate endpoint (like at pokeapi.co/api/v2/pokemon-form-trigger)? Maybe we could even have it list the pokemon forms that use that trigger (similar to a location area listing the encounters that linked it)? These suggestions may very well be something we handle in a separate PR and merge this one for now, but I thought I'd bring it up in case it was easy enough to work into this one. |
| @@ -0,0 +1,7 @@ | |||
| id,identifier | |||
There was a problem hiding this comment.
Personally I think we should make a new endpoint for clarity, as there are evolution triggers that wouldn’t work as form triggers
There was a problem hiding this comment.
I think there's a clear distinction between form changes and evolution. Form changes are temporary, while evolution is permanent. While that might not be a perfect definition, we should try to separate the two situations as much as possible.
3 participants
Change description
Addressing issues #1549 by adding new form triggers fields in
/api/v2/pokemon-form/endpoints.Changes included:
PokemonFormMethodandPokemonFormConditionmodels to store how a form is activated,pokemon_form_methods.csvto define trigger types, andpokemon_form_conditions.csvto link forms to their trigger methods.items.csvAI coding assistance disclosure
I use AI assistant for formatting.
Contributor check list